ES|QL SAMPLE aggregation function #127629

jan-elastic · 2025-05-02T10:16:58Z

No description provided.

elasticsearchmachine · 2025-05-02T13:32:37Z

Pinging @elastic/ml-core (Team:ML)

elasticsearchmachine · 2025-05-02T13:32:38Z

Hi @jan-elastic, I've created a changelog YAML for you.

...te/src/main/generated-src/org/elasticsearch/compute/aggregation/SampleBooleanAggregator.java

nik9000 · 2025-05-02T15:44:11Z

...in/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Sample.java

+            "version" },
+        description = "Collects sample values for a field.",
+        type = FunctionType.AGGREGATE,
+        examples = @Example(file = "stats_sample", tag = "doc")


I think we should have an example of the output in the docs. I'm not entirely sure the right way to hack that one up because it's non-deterministic. Maybe it's hand rolled.

I think we want that example because my first question when reading this is "can I get duplicates or do those count as distinct samples?" Mostly because I'm not good at statistics.

I do think it's interesting that SAMPLE(bool) is strictly more work than VALUES(bool). It feels like sampling shouldn't be, but it makes some sense.

I think it makes sense that SAMPLE(bool) is more work. VALUES(bool) just keeps track of two boolean values: does true exist and does false exist. SAMPLE(bool) does more.

Obv, I prefer some example output too. I didn't know how to achieve that, but I'll think of something. Should've left a TODO.

OK, I've hacked up something. Not particularly proud of it, but it gets the job done.

jan-elastic · 2025-05-06T11:09:35Z

.../esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-SampleAggregator.java.st

+            this.breaker = bigArrays.breakerService().getBreaker(CircuitBreaker.REQUEST);
+            this.sort = new BytesRefBucketedSort(breaker, "sample", bigArrays, SortOrder.ASC, limit);
+            this.bytesRefBuilder = new BreakingBytesRefBuilder(breaker, "sample");
+            this.random = new SplittableRandom();


Some notes on this SplittableRandom:

If I replace it by Random, I get the precommit error

Forbidden method invocation: java.util.Random#() [Use org.elasticsearch.common.Randomness#get for reproducible sources of randomness]

Using SplittableRandom instead works around this by not being on the blacklist, but that's not in the spirit of what's intended.

If I replace it by Randomness.get() in the constructor, I get:

java.lang.IllegalStateException: This Random was created for/by another thread (Thread[#39,TEST-SampleLongAggregatorFunctionTests.testManyInitialManyPartialFinalRunner-seed#[B3B51719A90700AD],5,TGRP-SampleLongAggregatorFunctionTests]). Random instances must not be shared (acquire per-thread). Current thread: Thread[#51,elasticsearch[test][esql_test_executor][T#1],5,TGRP-SampleLongAggregatorFunctionTests]

Even though the Aggregator is used on a single thread (I hope; otherwise there are more issues), it's created on a different thread then the thread that's actively using it.

If I replace it by Randomness.get() inside the add method, the test SampleLongAggregatorFunctionTests::testDistribution fails. It looks like each iteration instantiates the same random generator (same seed), leading to the statistics being completely wrong.

I'm still looking into these issues. If you have any thoughts, let me know.

OK! The Lucene stuff has stuff like:

if (Thread.currentThread() != prevThread) { prevThread = Thread.currentThread(); random = Randomness.get(); }

That might do.

Not sure what you exactly mean by this. Where's this stuff exactly?

I also whipped up a different fix. Let me know what you think...

ivancea

LGTM! Added some questions and things to check 👀

...in/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Sample.java

...c/test/java/org/elasticsearch/compute/aggregation/SampleBytesRefAggregatorFunctionTests.java

.../esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-SampleAggregator.java.st

elasticsearchmachine · 2025-05-07T12:43:31Z

Pinging @elastic/kibana-esql (ES|QL-ui)

ivancea

🚀

test/framework/src/main/java/org/elasticsearch/test/MixWithIncrement.java

.../esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-SampleAggregator.java.st

* ES|QL SAMPLE aggregation function * [CI] Auto commit changes from spotless * ThreadLocalRandom -> SplittableRandom * Update docs/changelog/127629.yaml * fix yaml test * Add SampleTests * docs + example * polish code * mark generated imports * comment with algorith description * use Randomness.get() * close properly * type checks * reuse hash * regen some files * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>

* ES|QL SAMPLE aggregation function (#127629) * ES|QL SAMPLE aggregation function * [CI] Auto commit changes from spotless * ThreadLocalRandom -> SplittableRandom * Update docs/changelog/127629.yaml * fix yaml test * Add SampleTests * docs + example * polish code * mark generated imports * comment with algorith description * use Randomness.get() * close properly * type checks * reuse hash * regen some files * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]> * Fix + unmute SampleTests (#127959) * Fix memory tracking of ES|QL sample agg (#128467) * Fix memory tracking of ES|QL sample agg * [CI] Auto commit changes from spotless * polish code --------- Co-authored-by: elasticsearchmachine <[email protected]> * ESQL: Unclean generated imports (#127723) This removes a ton of the tricky juggling we do for generated java files to keep the imports in order. Instead, we just live with them being out of order a little. It's not great, but it's so so so much easier than the terrible juggling we had been doing. * ESQL: Disable format checks on generated imports (#127648) This builds the infrastructure to disable spotless and some checkstyle rules on generated imports. This works around the most frustrating part of ESQL's string template generated files - the imports. It allows unused and out of order imports. This can let us remove a lot of cumbersome, tricky, and fairly useless `$if$` blocks from the templates. --------- Co-authored-by: elasticsearchmachine <[email protected]> Co-authored-by: Nik Everett <[email protected]>

leemthompo · 2025-07-23T14:54:43Z

@jan-elastic just a reminder here, that we'll need to add applies_to tags to this function eventually when it gets added per README.md#version-differentiation-in-docs-v3 :)

jan-elastic · 2025-07-23T15:29:21Z

@leemthompo Are you fixing this as well together with the other fixes? Thanks!

leemthompo · 2025-07-23T16:11:09Z

@jan-elastic yup sure can do, what's the correct availability info for this?

jan-elastic · 2025-07-24T06:45:01Z

@jan-elastic yup sure can do, what's the correct availability info for this?

Thanks!

The SAMPLE command did not exist in 9.0, and is GA in 9.1.

* Add applies to to ScalB function in #127696 * Add applies_to to categorize, follow up to #129398 * Add version info, following #127629 * SAMPLE is new + GA in 9.1 #127629 * add applies to for 9.2 option

* Add applies to to ScalB function in elastic#127696 * Add applies_to to categorize, follow up to elastic#129398 * Add version info, following elastic#127629 * SAMPLE is new + GA in 9.1 elastic#127629 * add applies to for 9.2 option (cherry picked from commit 5d565b5) # Conflicts: # docs/reference/query-languages/esql/_snippets/functions/parameters/categorize.md # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Categorize.java

* Add applies to to ScalB function in #127696 * Add applies_to to categorize, follow up to #129398 * Add version info, following #127629 * SAMPLE is new + GA in 9.1 #127629 * add applies to for 9.2 option (cherry picked from commit 5d565b5) # Conflicts: # docs/reference/query-languages/esql/_snippets/functions/parameters/categorize.md # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Categorize.java

* Add applies to to ScalB function in elastic#127696 * Add applies_to to categorize, follow up to elastic#129398 * Add version info, following elastic#127629 * SAMPLE is new + GA in 9.1 elastic#127629 * add applies to for 9.2 option

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 labels May 2, 2025

jan-elastic force-pushed the esql-sample-agg-2 branch from 55fe96f to 62de767 Compare May 2, 2025 13:23

jan-elastic added >feature :ml Machine learning Team:ML Meta label for the ML team labels May 2, 2025

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label May 2, 2025

jan-elastic requested a review from alex-spies May 2, 2025 15:36

nik9000 reviewed May 2, 2025

View reviewed changes

jan-elastic force-pushed the esql-sample-agg-2 branch 2 times, most recently from 717536b to 3c4bae7 Compare May 6, 2025 10:42

jan-elastic commented May 6, 2025

View reviewed changes

jan-elastic force-pushed the esql-sample-agg-2 branch from 3c4bae7 to d46b18f Compare May 6, 2025 11:21

jan-elastic requested a review from nik9000 May 6, 2025 12:20

jan-elastic force-pushed the esql-sample-agg-2 branch from d46b18f to 520087d Compare May 6, 2025 14:51

jan-elastic requested review from ivancea and removed request for alex-spies May 7, 2025 08:17

ivancea reviewed May 7, 2025

View reviewed changes

jan-elastic force-pushed the esql-sample-agg-2 branch from 520087d to 940b6f4 Compare May 7, 2025 12:20

jan-elastic requested a review from ivancea May 7, 2025 12:26

jan-elastic added the ES|QL-ui Impacts ES|QL UI label May 7, 2025

ivancea approved these changes May 7, 2025

View reviewed changes

test/framework/src/main/java/org/elasticsearch/test/MixWithIncrement.java Outdated Show resolved Hide resolved

jan-elastic and others added 5 commits May 7, 2025 16:13

ES|QL SAMPLE aggregation function

534ae36

[CI] Auto commit changes from spotless

6441c8e

ThreadLocalRandom -> SplittableRandom

3dbc067

Update docs/changelog/127629.yaml

6f82570

fix yaml test

67676a1

jan-elastic added 6 commits May 7, 2025 16:13

comment with algorith description

594cc13

use Randomness.get()

1ff7ab0

close properly

06c8d46

type checks

7b12699

reuse hash

7325dac

regen some files

c1dc208

jan-elastic force-pushed the esql-sample-agg-2 branch from 865c90c to c1dc208 Compare May 7, 2025 14:13

jan-elastic requested a review from a team as a code owner May 7, 2025 14:13

nik9000 approved these changes May 7, 2025

View reviewed changes

.../esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-SampleAggregator.java.st Show resolved Hide resolved

[CI] Auto commit changes from spotless

632a23d

jan-elastic merged commit 9cf2a64 into elastic:main May 8, 2025
17 checks passed

jan-elastic deleted the esql-sample-agg-2 branch May 8, 2025 06:02

leemthompo added a commit to leemthompo/elasticsearch that referenced this pull request Jul 24, 2025

Add version info, following elastic#127629

7c3dc70

leemthompo added a commit to leemthompo/elasticsearch that referenced this pull request Jul 24, 2025

SAMPLE is new + GA in 9.1 elastic#127629

49783f1

leemthompo mentioned this pull request Jul 24, 2025

[DOCS] Update ES|QL applies to's #131805

Merged

ES|QL SAMPLE aggregation function #127629

ES|QL SAMPLE aggregation function #127629

Uh oh!

Conversation

jan-elastic commented May 2, 2025

Uh oh!

elasticsearchmachine commented May 2, 2025

Uh oh!

elasticsearchmachine commented May 2, 2025

Uh oh!

Uh oh!

nik9000 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

jan-elastic May 6, 2025

Choose a reason for hiding this comment

Uh oh!

jan-elastic May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jan-elastic May 6, 2025

Choose a reason for hiding this comment

Uh oh!

jan-elastic May 6, 2025

Choose a reason for hiding this comment

Uh oh!

nik9000 May 6, 2025

Choose a reason for hiding this comment

Uh oh!

jan-elastic May 7, 2025

Choose a reason for hiding this comment

Uh oh!

jan-elastic May 7, 2025

Choose a reason for hiding this comment

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented May 7, 2025

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leemthompo commented Jul 23, 2025

Uh oh!

jan-elastic commented Jul 23, 2025

Uh oh!

leemthompo commented Jul 23, 2025

Uh oh!

jan-elastic commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jan-elastic May 6, 2025 •

edited

Loading

jan-elastic commented Jul 24, 2025 •

edited

Loading